Skip to content

Better LLM Explanations CF-597 #125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 19 commits into from
Jul 31, 2025
Merged

Better LLM Explanations CF-597 #125

merged 19 commits into from
Jul 31, 2025

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented Apr 4, 2025 •
edited by github-actions bot
Loading

User description

wip

PR Type

Enhancement

Description

  • Add get_new_explanation method in AIService

  • Integrate explanation API call in function optimizer

  • Construct fallback logic for explanation

  • Handle request errors and log appropriately

Diagram Walkthrough

flowchart LR
  A["Source and optimized code"] -- "payload to /explain" --> B["AIService.get_new_explanation"]
  B -- "returns explanation" --> C["new_explanation object"]
  C -- "used in" --> D["check_create_pr"]
Loading

File Walkthrough
Relevant files
Enhancement
aiservice.py
Implement explanation endpoint in AIService                           

codeflash/api/aiservice.py

  • Implement get_new_explanation method
  • Compose payload with code and profiling data
  • Handle request exceptions and HTTP errors
  • Log responses and fallback empty explanation
 +68/-1   
function_optimizer.py
Integrate AI explanations into optimizer                                 

codeflash/optimization/function_optimizer.py

  • Invoke get_new_explanation in optimizer workflow
  • Import dataclasses.replace for Explanation
  • Build new_explanation with fallback raw message
  • Pass updated explanation to check_create_pr
 +22/-2   

@aseembits93 aseembits93 marked this pull request as draft April 4, 2025 22:01
@github-actions
Copy link

github-actions bot commented Apr 4, 2025 •
edited
Loading

PR Reviewer Guide 🔍

(Review updated until commit 08aed91)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Incorrect Docstring

The docstring for get_new_explanation refers to optimization parameters and return types unrelated to explanation, which can confuse maintainers.

 
"""Optimize the given python code for performance by making a request to the Django endpoint.

Parameters
----------
- source_code (str): The python code to optimize.
- dependency_code (str): The dependency code used as read-only context for the optimization
- trace_id (str): Trace id of optimization run
- num_candidates (int): Number of optimization variants to generate. Default is 10.
- experiment_metadata (Optional[ExperimentalMetadata, None]): Any available experiment metadata for this optimization
- existing_explanation (str): Existing explanation from AIservice call

Returns
-------
- List[OptimizationCandidate]: A list of Optimization Candidates.

"""
Unused Import

The replace import from dataclasses is not used and should be removed to keep the code clean.

 
from dataclasses import replace
Missing Import

The code constructs a new Explanation but there is no visible import for Explanation in this file, which may cause a NameError.

 
new_explanation = Explanation(raw_explanation_message=new_explanation_raw_str or explanation.raw_explanation_message, winning_behavior_test_results=explanation.winning_behavior_test_results,
                                winning_benchmarking_test_results=explanation.winning_benchmarking_test_results,
                                original_runtime_ns=explanation.original_runtime_ns,
                                best_runtime_ns=explanation.best_runtime_ns,
                                function_name=explanation.function_name,
                                file_path=explanation.file_path,
                                benchmark_details=explanation.benchmark_details)

@github-actions
Copy link

github-actions bot commented Apr 4, 2025 •
edited
Loading

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Safely parse JSON response

Use safe JSON parsing and default values when extracting explanation to avoid
KeyError or JSON decode errors. Wrap the .json() call in a try/except and use .get
with a default.

codeflash/api/aiservice.py [333-334]

 if response.status_code == 200:
-    explanation = response.json()["explanation"]
+    try:
+        resp_json = response.json()
+        explanation = resp_json.get("explanation", "")
+    except ValueError:
+        explanation = ""
Suggestion importance[1-10]: 6

__

Why: Using try/except and .get prevents unhandled KeyError or JSON parsing errors in the 200-case, improving robustness with moderate impact.

Low

@aseembits93 aseembits93 marked this pull request as ready for review July 30, 2025 19:41
@github-actions
Copy link

Persistent review updated to latest commit 08aed91

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Safely parse JSON response

Use safe JSON parsing and default values when extracting explanation to avoid
KeyError or JSON decode errors. Wrap the .json() call in a try/except and use .get
with a default.

codeflash/api/aiservice.py [333-334]

 if response.status_code == 200:
-    explanation = response.json()["explanation"]
+    try:
+        resp_json = response.json()
+        explanation = resp_json.get("explanation", "")
+    except ValueError:
+        explanation = ""
Suggestion importance[1-10]: 6

__

Why: Using try/except and .get prevents unhandled KeyError or JSON parsing errors in the 200-case, improving robustness with moderate impact.

Low

KRRT7
KRRT7 reviewed Jul 30, 2025
View reviewed changes
KRRT7
KRRT7 previously approved these changes Jul 30, 2025
View reviewed changes
misrasaurabh1
misrasaurabh1 reviewed Jul 30, 2025
View reviewed changes
misrasaurabh1
misrasaurabh1 reviewed Jul 30, 2025
View reviewed changes
misrasaurabh1
misrasaurabh1 reviewed Jul 30, 2025
View reviewed changes
misrasaurabh1
misrasaurabh1 reviewed Jul 30, 2025
View reviewed changes
aseembits93
aseembits93 commented Jul 30, 2025
View reviewed changes
Copy link
Contributor Author

@aseembits93 aseembits93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

incorporated all the feedback

@aseembits93 aseembits93 requested a review from Saga4 July 31, 2025 22:22
@aseembits93 aseembits93 enabled auto-merge July 31, 2025 22:27
@aseembits93 aseembits93 merged commit d5adf4b into main Jul 31, 2025
18 checks passed
@codeflash-ai codeflash-ai bot mentioned this pull request Jul 31, 2025
Closed
@aseembits93 aseembits93 deleted the llm-explanations branch August 17, 2025 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@misrasaurabh1 misrasaurabh1 misrasaurabh1 approved these changes

@KRRT7 KRRT7 KRRT7 left review comments

@mohammedahmed18 mohammedahmed18 Awaiting requested review from mohammedahmed18

@Saga4 Saga4 Awaiting requested review from Saga4

Assignees

No one assigned

Labels

Review effort 3/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aseembits93 @misrasaurabh1 @KRRT7